The changes make sense to me. I guess there are some more weirdnesses around focusing the main view in half or full screen mode, but I don't use these modes much, and can't be bothered right now to explore this more. 😄

We have fancy tests for this kind of stuff in window_arrangement_helper_test.go, do you think it's worth adding tests for these new conditions?

We have fancy tests for this kind of stuff in window_arrangement_helper_test.go, do you think it's worth adding tests for these new conditions?

Sure, just added! Those tests don't end up exercising the crash point that created this PR, so I also made an integration test for that behavior in particular.

I also realized I had inadvertently duplicated the logic of "make the main view full screen", so I added a fixup to simply that logic.

Looks great. Just one last question about the integration test: I verified that it crashes when I revert the production code changes, but only if I run it with go test pkg/integration/clients/*.go. If I run it with go run cmd/integration_test/main.go cli ui/side_panel_width_1, then it succeeds. Any idea why that might be?

Hmmm, I'm unable to reproduce that. If I cherrypick the test out to master, and run it with:

(cmd)chrismcdonnell@Chris-Laptop:~/linux-repos/lazygit$ go run cmd/integration_test/main.go cli ui/side_panel_width_1
panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/jesseduffield/lazycore/pkg/boxlayout.normalizeWeights({0xc000414300, 0x2, 0x2})
        /home/chrismcdonnell/linux-repos/lazygit/vendor/github.com/jesseduffield/lazycore/pkg/boxlayout/boxlayout.go:161 +0x46f
github.com/jesseduffield/lazycore/pkg/boxlayout.calcSizes({0xc000426db0, 0x2, 0x0?}, 0xbe)
        /home/chrismcdonnell/linux-repos/lazygit/vendor/github.com/jesseduffield/lazycore/pkg/boxlayout/boxlayout.go:97 +0x65
github.com/jesseduffield/lazycore/pkg/boxlayout.ArrangeWindows(0xc0004662d0, 0x0, 0x0, 0xbe, 0x2a)

I get the error. I also squashed the 3 commits and reverted them, and then it also failed there

I investigated a bit more, and found that it depends on the window geometry: if the window is twice as wide as it is tall, the test succeeds; if it is square, it crashes.

Playing around with it more, I'm not sure the behavior is always what we want when you are in staging or in patch building mode. I need more time to wrap my head around it, and probably won't get around to it this weekend; but I'd like to hold off merging this until I understand this better.

Ok, so there are problems because of this code: it runs unconditionally no matter whether we are in half-screen or full-screen mode, and this causes weird behavior, depending on the window geometry. For example, if you select a directory in the files panel that has both staged and unstaged changes, then it will show the main panels even with sidePanelWidth: 1 if the window is wide enough (because if it is, we split the main panels side-by-side), or if you set gui.mainPanelSplitMode: horizontal explicitly. I suppose this is also the reason why that integration test failed or succeeded depending on how wide the window is.

I think this condition needs to move somehow so that it is only checked when we're not in full or half screen mode, and only if mainSectionWeight is not 1. I don't have enough energy right now to make a concrete suggestion what this could look like.

Other observations:

• with sidePanelWidth: 1, you can't see the command log panel even if you focus it. I guess it needs a similar condition as the main views when they are focused.
• with sidePanelWidth: 0, lazygit crashes on startup. Probably not something that anybody would do, but might still be nice to be graceful about it, somehow (maybe just by clamping to a known good minimal value).